Core: Pick inner most parse exception as root cause #30270
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Just like `ElasticsearchException`, the inner most `XContentParseException` tends to contain the root cause of the exception and show be show to the user in the `root_cause` field. The effectively undoes most of the changes that elastic#29373 made to the `root_cause` for parsing exceptions. The `type` field still changes from `parse_exception` to `x_content_parse_exception`, but this seems like a fairly safe change. `ElasticsearchWrapperException` *looks* tempting to implement this but the behavior isn't quite right. `ElasticsearchWrapperExceptions` are entirely unwrapped until the cause no longer `implements ElasticsearchWrapperException` but `XContentParseException` should be unwrapped until its cause is no longer an `XContentParseException` but no further. In other words, `ElasticsearchWrapperException` are unwrapped one step too far. Closes elastic#30261
nik9000
added
>bug
review
:Core/Infra/Core
|
Pinging @elastic/es-core-infra |
nik9000
commented
Apr 30, 2018
| return ((ElasticsearchException) ex).guessRootCauses(); | ||
| } | ||
| if (ex instanceof XContentParseException) { | ||
| /* |
There was a problem hiding this comment.
I feel like if we get another exception like this one we should try to build something a little more generic and less "heuristics that make the exception look good". But XContentParseException isn't really the same thing as ElasticsearchException`. It is similar, but different enough that I don't think I could boil out the generic bits without this getting wonky.
| @@ -19,7 +19,11 @@ | |||
|
|
|||
| package org.elasticsearch; | |||
|
|
|||
| /** | |||
| * An exception that is meant to be "unwrapped" when sent back to the user | |||
There was a problem hiding this comment.
This one turned out not to be useful but I researched it so I figured I'd add the javadocs.
| @@ -124,13 +126,13 @@ public void testGuessRootCause() { | |||
| } else { | |||
| rootCauses = ElasticsearchException.guessRootCauses(randomBoolean() ? new RemoteTransportException("remoteboom", ex) : ex); | |||
| } | |||
| assertEquals(ElasticsearchException.getExceptionName(rootCauses[0]), "parsing_exception"); | |||
There was a problem hiding this comment.
These were all backwards....
|
Thanks for reviewing @dakrone! I've merged and I'll backport now! |
nik9000
added a commit
that referenced
this pull request
May 1, 2018
Just like `ElasticsearchException`, the inner most `XContentParseException` tends to contain the root cause of the exception and show be show to the user in the `root_cause` field. The effectively undoes most of the changes that #29373 made to the `root_cause` for parsing exceptions. The `type` field still changes from `parse_exception` to `x_content_parse_exception`, but this seems like a fairly safe change. `ElasticsearchWrapperException` *looks* tempting to implement this but the behavior isn't quite right. `ElasticsearchWrapperExceptions` are entirely unwrapped until the cause no longer `implements ElasticsearchWrapperException` but `XContentParseException` should be unwrapped until its cause is no longer an `XContentParseException` but no further. In other words, `ElasticsearchWrapperException` are unwrapped one step too far. Closes #30261
nik9000
added a commit
that referenced
this pull request
May 1, 2018
Just like `ElasticsearchException`, the inner most `XContentParseException` tends to contain the root cause of the exception and show be show to the user in the `root_cause` field. The effectively undoes most of the changes that #29373 made to the `root_cause` for parsing exceptions. The `type` field still changes from `parse_exception` to `x_content_parse_exception`, but this seems like a fairly safe change. `ElasticsearchWrapperException` *looks* tempting to implement this but the behavior isn't quite right. `ElasticsearchWrapperExceptions` are entirely unwrapped until the cause no longer `implements ElasticsearchWrapperException` but `XContentParseException` should be unwrapped until its cause is no longer an `XContentParseException` but no further. In other words, `ElasticsearchWrapperException` are unwrapped one step too far. Closes #30261
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Just like
ElasticsearchException, the inner mostXContentParseExceptiontends to contain the root cause of theexception and show be show to the user in the
root_causefield.The effectively undoes most of the changes that #29373 made to the
root_causefor parsing exceptions. Thetypefield still changes fromparse_exceptiontox_content_parse_exception, but this seems like afairly safe change.
ElasticsearchWrapperExceptionlooks tempting to implement this butthe behavior isn't quite right.
ElasticsearchWrapperExceptionsareentirely unwrapped until the cause no longer
implements ElasticsearchWrapperExceptionbutXContentParseExceptionshould be unwrapped until its cause is no longer an
XContentParseExceptionbut no further. In other words,ElasticsearchWrapperExceptionare unwrapped one step too far.Closes #30261